Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement missing methods #422

Merged
merged 4 commits into from
Mar 18, 2016
Merged

Implement missing methods #422

merged 4 commits into from
Mar 18, 2016

Conversation

hynek
Copy link
Contributor

@hynek hynek commented Feb 13, 2016

First shot at #388.

  • It would be nice if @tiran (or anyone else understand the matter) could suggest a better docstring.
  • If anyone knows what could be checked in the test case I’m all ears. There doesn’t seem to be a get_session_id?
  • We also need to find a way to simulate errors. :-/

N.B. only the -cryptographyMaster envs are supposed to pass. Once this and the other missing methods are signed off, @reaperhulk will release a cryptography 1.2 point release with the necessary bindings.

  • set cryptography requirement to >=1.3 in setup.py once 1.3 is available.

buf,
len(buf),
) != 1:
_raise_current_error()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get around the lack of coverage for this case by making a new function:

def _pyopenssl_assert(ok):
    if not ok:
        _raise_current_error()

then you can change the above to

        res = _lib.SSL_CTX_set_session_id_context(
                self._context,
                buf,
                len(buf),
        )
        _pyopenssl_assert(res == 1)

Coverage on _pyopenssl_assert is, of course, trivial then. This is the approach we use in cryptography when we have response codes we expect to always be a specific value unless something catastrophic has occurred.

Edit: I meant res == 1. Comment updated ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, in this case you could trigger the error by passing a session ID context value greater than SSL_MAX_SSL_SESSION_ID_LENGTH, which appears to be 32 (bytes?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done! I’ve taken a, much higher number in the hopes that this number won't change significantly in the next OpenSSL release…

@hynek
Copy link
Contributor Author

hynek commented Feb 14, 2016

(FYI as soon as this is signed off [but no sooner], I’ll tackle the next one. It would be nice tho if someone else could start working on them too.)

@hynek hynek changed the title [WIP] Implement Context.set_session_id [WIP] Implement missing methods Feb 27, 2016
@hynek
Copy link
Contributor Author

hynek commented Feb 27, 2016

FYI I’ll fix #387 and #305 here too. It’s cumbersome to work on them separately.

@hynek hynek changed the title [WIP] Implement missing methods Implement missing methods Feb 27, 2016
@hynek
Copy link
Contributor Author

hynek commented Feb 27, 2016

Relevant builders are green.

Please tear it apart so we can have a 16.0.0 soon!

@alex
Copy link
Member

alex commented Mar 11, 2016

@hynek this needs a rebase

@hynek
Copy link
Contributor Author

hynek commented Mar 11, 2016

done, all relevant builds are green.

# """
# """
# server, client = self._loopback()
def test_renegotiate(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this test call renegotiate_pending during and after to confirm it properly returns True/False at the expected time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve added some asserts and the builders are still green.

reaperhulk added a commit that referenced this pull request Mar 13, 2016
Pluck more unrelated bits from #422
@codecov-io
Copy link

Current coverage is 87.95%

Merging #422 into master will increase coverage by +0.11% as of 6d16da2

@@            master    #422   diff @@
======================================
  Files            7       7       
  Stmts         2032    2042    +10
  Branches       372     369     -3
  Methods          0       0       
======================================
+ Hit           1785    1796    +11
+ Partial        117     115     -2
- Missed         130     131     +1

Review entire Coverage Diff as of 6d16da2

Powered by Codecov. Updated on successful CI builds.

reaperhulk added a commit that referenced this pull request Mar 18, 2016
@reaperhulk reaperhulk merged commit 9dff5c4 into pyca:master Mar 18, 2016
@glyph
Copy link
Contributor

glyph commented Mar 19, 2016

This is amazing and all you guys are awesome.

jsonn referenced this pull request in jsonn/pkgsrc Apr 20, 2016
Changes:
16.0.0 (2016-03-19)
-------------------
This is the first release under full stewardship of PyCA.
We have made *many* changes to make local development more pleasing.
The test suite now passes both on Linux and OS X with OpenSSL 0.9.8,
1.0.1, and 1.0.2.  It has been moved to `py.test <https://pytest.org/>`_,
all CI test runs are part of `tox <https://testrun.org/tox/>`_ and
the source code has been made fully `flake8
<https://flake8.readthedocs.org/>`_ compliant.

We hope to have lowered the barrier for contributions significantly
but are open to hear about any remaining frustrations.

Backward-incompatible changes:
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Python 3.2 support has been dropped.
  It never had significant real world usage and has been dropped
  by our main dependency ``cryptography``.  Affected users should
  upgrade to Python 3.3 or later.

Deprecations:
^^^^^^^^^^^^^
- The support for EGD has been removed.
  The only affected function ``OpenSSL.rand.egd()`` now uses
  ``os.urandom()`` to seed the internal PRNG instead.  Please see
  `pyca/cryptography#1636
  <https://github.com/pyca/cryptography/pull/1636>`_ for more
  background information on this decision.  In accordance with our
  backward compatibility policy ``OpenSSL.rand.egd()`` will be
  *removed* no sooner than a year from the release of 16.0.0.
  Please note that you should `use urandom
  <http://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/>`_
  for all your secure random number needs.
- Python 2.6 support has been deprecated.
  Our main dependency ``cryptography`` deprecated 2.6 in version
  0.9 (2015-05-14) with no time table for actually dropping it.
  pyOpenSSL will drop Python 2.6 support once ``cryptography``
  does.

Changes:
^^^^^^^^
- Fixed ``OpenSSL.SSL.Context.set_session_id``,
  ``OpenSSL.SSL.Connection.renegotiate``,
  ``OpenSSL.SSL.Connection.renegotiate_pending``, and
  ``OpenSSL.SSL.Context.load_client_ca``.
  They were lacking an implementation since 0.14.  `#422
  <https://github.com/pyca/pyopenssl/pull/422>`_
- Fixed segmentation fault when using keys larger than 4096-bit to sign data.
  `#428 <https://github.com/pyca/pyopenssl/pull/428>`_
- Fixed ``AttributeError`` when ``OpenSSL.SSL.Connection.get_app_data()``
  was called before setting any app data.
  `#304 <https://github.com/pyca/pyopenssl/pull/304>`_
- Added ``OpenSSL.crypto.dump_publickey()`` to dump ``OpenSSL.crypto.PKey``
  objects that represent public keys, and ``OpenSSL.crypto.load_publickey()``
  to load such objects from serialized representations.
  `#382 <https://github.com/pyca/pyopenssl/pull/382>`_
- Added ``OpenSSL.crypto.dump_crl()`` to dump a certificate revocation
  list out to a string buffer.
  `#368 <https://github.com/pyca/pyopenssl/pull/368>`_
- Added ``OpenSSL.SSL.Connection.get_state_string()`` using the
  OpenSSL binding ``state_string_long``.
  `#358 <https://github.com/pyca/pyopenssl/pull/358>`_
- Added support for the ``socket.MSG_PEEK`` flag to
  ``OpenSSL.SSL.Connection.recv()`` and
  ``OpenSSL.SSL.Connection.recv_into()``.
  `#294 <https://github.com/pyca/pyopenssl/pull/294>`_
- Added ``OpenSSL.SSL.Connection.get_protocol_version()`` and
  ``OpenSSL.SSL.Connection.get_protocol_version_name()``.
  `#244 <https://github.com/pyca/pyopenssl/pull/244>`_
- Switched to ``utf8string`` mask by default.
  OpenSSL formerly defaulted to a ``T61String`` if there were UTF-8
  characters present.  This was changed to default to ``UTF8String``
  in the config around 2005, but the actual code didn't change it
  until late last year.  This will default us to the setting that
  actually works.  To revert this you can call
  ``OpenSSL.crypto._lib.ASN1_STRING_set_default_mask_asc(b"default")``.
  `#234 <https://github.com/pyca/pyopenssl/pull/234>`_
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants